Skip to content

fix(automation): a fault edge must not switch off a guardrail (#3863) - #3881

Merged
os-zhuang merged 1 commit into
mainfrom
claude/flow-node-error-branches
Jul 28, 2026
Merged

fix(automation): a fault edge must not switch off a guardrail (#3863)#3881
os-zhuang merged 1 commit into
mainfrom
claude/flow-node-error-branches

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

What this turned out to be

#3863 was filed to build node-level error branches. On opening the engine, they already exist and work: type: 'fault' is in the edge schema, executeNode routes both the thrown and the returned failure paths to the handler, $error is populated, and there are tests. So the feature was not the gap.

The gap was that it routed everything — including guard refusals. That is the fail-open hole flagged as the hard design constraint when the issue was promoted, and it is live in main today.

The bug, reproduced before fixing

Attach a fault edge to a delete_record whose filter has a typo:

{ id: 'op', type: 'delete_record',
  config: { objectName: 'deal', filter: { owner: '{record.ownr}' } } },   // typo
{ id: 'e_fault', source: 'op', target: 'handler', type: 'fault' },

{record.ownr} does not resolve, so the only condition is erased and the filter would collapse to {} — every row. #3810's guard correctly refuses the node. The fault edge then swallowed the refusal, ran the handler, and the run reported success: true.

One declared edge silently disabled the platform's protection against emptying an object. That is precisely the fail-open direction #3810 was opened to close, and it is the first thing an AI authoring loop reaches for when trying to make a diagnostic go away — the same suppression dynamic that got the $optional proposal (#3862) closed as not planned.

The test asserting this lands in the tree failing, then passes with the fix.

The fix: failures carry a class

NodeExecutionResult.errorClass'runtime' (default) or 'guard'.

Class Examples Routable by a fault edge
runtime http 404, connector rate-limit, data engine rejected a write yes
guard filter lost a condition (#3810), data node names no object, run would execute unscoped (ADR-0049/#1888) no — stays fatal

Defaulting to runtime means every existing executor keeps its current routing; this is not a behaviour change for anything except the guards.

Thrown guards are covered too. resolveNodeFilter returns its refusal; UnscopedRunDataAccessError throws one. Both paths are contained, via a shared guard-refusal module (its own file because engine.ts already imports runtime-identity.ts, so the guard cannot import the engine back). Otherwise the catch path would simply become the bypass the return path no longer is.

Marked guard-class: the three resolveNodeFilter refusals, the four objectName required refusals, and UnscopedRunDataAccessError. Genuine engine failures (get_record(x) failed: …) stay runtime-class and keep routing.

Also in scope (the rest of the issue's v1)

Verification

  • New fault-edge-guard-containment.test.ts — 6 tests: guard refusal contained with a fault edge present; runtime failure on the same shape still routes; thrown ADR-0049 guard also contained; guard fatal without a fault edge; {<nodeId>.error} published; failed step still traced after recovery.
  • packages/services/service-automation full suite: 407 passed, no regressions — the pre-existing fault-edge tests still pass, which is the point of defaulting to runtime.
  • ESLint clean on all changed files; check-role-word gate OK.
  • tsc --noEmit on the package reports 2 errors, both in engine.test.ts about resumeAuthority — confirmed pre-existing by stashing this branch's changes and re-running on the base.

Deferred (still tracked on #3863)

Interaction with flow-level errorHandling.retry; error edges that loop back (needs the ADR-0044 type: 'back' cycle treatment); whether label: 'error' should be an accepted alias.


Generated by Claude Code

A `fault` edge routes a failed node to a handler instead of aborting the run —
the right primitive for the world not cooperating (a 404, a rate-limit, a
rejected write).

It was also routing the refuse-to-execute family. Those guards report that the
METADATA is wrong, not that an operation failed: #3810 (interpolation erased a
filter condition), ADR-0049/#1888 (the run would execute unscoped), a data node
naming no object. Surfacing as ordinary node failures, one declared edge
silently disabled them.

Reproduced in a test before fixing: a `fault` edge on a `delete_record` whose
filter has a typo (`{record.ownr}`) made #3810's protection against emptying the
object vanish — guard fired, handler swallowed it, run reported success: true.
The exact fail-open direction #3810 was opened to close, reachable from one
edge, and the first thing an AI loop reaches for to make a diagnostic go away.

Failures now carry a class. `NodeExecutionResult.errorClass` is 'runtime'
(default, so every existing executor keeps its routing) or 'guard'. Guard-class
failures are never routed: fatal with or without a fault edge, failing with the
guard's own message. Thrown guards are covered too — UnscopedRunDataAccessError
is branded through a shared `guard-refusal` module, so the catch path cannot
become the bypass the return path no longer is.

Marked guard-class: three resolveNodeFilter refusals, four `objectName required`
refusals, UnscopedRunDataAccessError. Genuine engine failures stay runtime-class
and keep routing.

Also here:

- `{<nodeId>.error}` carries a failed node's message alongside run-wide
  `{$error}`, which names only the most recent failure — a handler shared by two
  fault edges could not tell which node it was handling. Additive.
- Fault edges are documented for the first time (flows.mdx + automation skill),
  including the routable/not-routable split and an explicit "do not add one to
  silence a guard error".

A run taking a fault branch still reports success, and the failed step keeps
status 'failure' and its message in the trace (#3356/#3407).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1
@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Jul 28, 2026 2:06pm

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling size/l labels Jul 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): packages/services.

6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/webhooks.mdx (via packages/services)
  • content/docs/kernel/runtime-services/audit-service.mdx (via packages/services)
  • content/docs/kernel/runtime-services/index.mdx (via packages/services)
  • content/docs/kernel/runtime-services/settings-service.mdx (via packages/services)
  • content/docs/plugins/packages.mdx (via packages/services)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/services)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuang
os-zhuang marked this pull request as ready for review July 28, 2026 14:15
@os-zhuang
os-zhuang merged commit 2f47489 into main Jul 28, 2026
16 checks passed
@os-zhuang
os-zhuang deleted the claude/flow-node-error-branches branch July 28, 2026 14:15
os-zhuang pushed a commit that referenced this pull request Jul 28, 2026
The #3881 callout in flows.mdx and the automation skill enumerated the guards
that existed at the time. This PR adds five more, which makes an enumeration the
wrong shape — it was already stale by the end of its own branch.

Both now state the TEST instead: a failure is a guard when re-running unchanged
could never succeed AND the fix is to edit metadata. The examples stay, as
examples rather than as the definition, and the runtime side gains the cases
this PR deliberately left routable (a degraded connector, a subflow that failed
on its own) so the two lists read as one contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1
os-zhuang added a commit that referenced this pull request Jul 28, 2026
…ute guards (#3863) (#3889)

#3881 stopped a `fault` edge swallowing a guard refusal, keyed on `errorClass`.
That field defaults to 'runtime' — right for compatibility, since every executor
written before the split keeps its routing — but it leaves the footgun pointing
the other way: a NEW guard is routable unless its author remembers to classify
it, and forgetting is silent.

`refuseNode(reason)` returns a guard-class failure, so writing a guard and
marking it un-routable become one act. Its doc states the test for using it
(re-running unchanged can never succeed AND the fix is to edit metadata) and the
inverse, because over-marking turns a recoverable integration into a dead run.

Five guards that were never marked are now un-routable — all missing required
config or a defective graph, none able to succeed on a retry:

  - `http` with no url
  - `subflow` with no flowName, and subflow past max nesting depth (a recursive
    graph nests exactly as deep next run)
  - `map` with no flowName
  - `connector_action` with no connectorId/actionId

The seven crud-nodes guards from #3881 move to the helper — same behaviour, one
spelling.

A behavioural inventory test drives every known guard through the engine with a
fault edge attached and asserts it stays fatal, matching the refusal text so a
guard failing for a different reason cannot pass vacuously. Verified to have
teeth: un-marking one fails its row immediately. The negative half is pinned
too — a plain failure and a thrown error must still route.

Deliberately not marked: a degraded connector (#3017 recovers automatically), a
collection that did not resolve to an array, a collection over the iteration
cap, a subflow that failed on its own. The world caused those.

The flows.mdx and skill callouts now state the guard/runtime TEST rather than a
closed list, which this PR had already made stale within its own branch.

Considered and rejected: making `errorClass` required on the result type —
compile-time enforcement, but it breaks 281 call sites plus third-party
executors for a type-only gain over the helper.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants